feat(loops): Run a skill instead of instructions - #73069
Conversation
|
😎 This pull request was merged. |
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
products/tasks/backend/facade/loops.py:1046
**Partial replacements leak uploads**
When an earlier bundle is uploaded and a later bundle fails validation or storage writing, the manifest update never runs and the earlier object remains unreferenced under the persistent loop prefix with no retention TTL or subsequent cleanup path.
### Issue 2 of 4
products/tasks/backend/logic/services/loop_runs.py:501
**Rollback leaves copied artifacts**
When one bundle copy succeeds before a later copy or manifest save fails, the database transaction rolls back but the copied object remains under the discarded run prefix. Temporal retries create new run IDs, so each failed attempt accumulates another set of unreachable objects.
### Issue 3 of 4
products/tasks/backend/facade/loops.py:1023-1027
**Concurrent replacements orphan bundles**
When two replacement requests overlap, both read the same previous manifest and upload separate objects without locking the loop. The last database write wins, leaving the other request's newly uploaded objects unreferenced and outside later manifest-based cleanup.
### Issue 4 of 4
products/tasks/backend/facade/loops.py:1046
**Deleted loops retain bundle objects**
When a loop with attached bundles is soft-deleted, these persistent loop-scoped objects are not removed or assigned a retention TTL, so deleting the loop leaves its bundle data stored indefinitely.
Reviews (1): Last reviewed commit: "add loop skill bundles seeded into fired..." | Re-trigger Greptile |
PR overviewThis PR changes loop execution so that a loop runs a skill rather than standalone instructions. The loop-run service snapshots the selected skill bundle when creating scheduled or teammate-triggered tasks. Six issues have been addressed, but one significant ownership-transition issue remains. A previous owner’s skill bundle can persist after a teammate takes ownership, allowing attacker-authored instructions to execute using the new owner’s credentials when the loop fires. Open issues (1)
Fixed/addressed: 6 · PR risk: 8/10 |
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.24 MiB · 22 files | no change | ███░░░░░░░ 27.5% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.07 MiB · 3,010 files | 🔺 +851 B (+0.0%) | ████████░░ 83.1% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.5 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 236.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 226.1 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/module.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.8 KiB | src/lib/api.ts |
| 94.0 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.18 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.18 MiB · 17 files | no change | ████░░░░░░ 38.1% of 5.72 MiB |
| Deferred (lazy) | 2.07 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 713.8 KiB | dist/toolbar/toolbar-app-HLTHZTTF.css |
| 545.0 KiB | dist/toolbar/chunk-chunk-JGF7ZYC3.js |
| 484.2 KiB | dist/toolbar/chunk-chunk-YMFODIKP.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-YSVTUFEA.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-Y3ZKEAFE.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-ALQOU6WO.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-VMBXRLPQ.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +15.6 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1352.23 MiB · 🔺 +15.6 KiB (+0.0%)
ℹ️ MCP UI apps size — 32 app(s), 17065.5 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 599.5 KB | 187.7 KB |
| action | 457.8 KB | 187.7 KB |
| action-list | 564.3 KB | 187.7 KB |
| cohort | 456.8 KB | 187.7 KB |
| cohort-list | 563.3 KB | 187.7 KB |
| email-template | 456.6 KB | 187.7 KB |
| error-details | 472.4 KB | 187.7 KB |
| error-issue | 457.5 KB | 187.7 KB |
| error-issue-list | 564.2 KB | 187.7 KB |
| experiment | 561.5 KB | 187.7 KB |
| experiment-list | 565.1 KB | 187.7 KB |
| experiment-results | 563.2 KB | 187.7 KB |
| feature-flag | 567.1 KB | 187.7 KB |
| feature-flag-list | 570.9 KB | 187.7 KB |
| feature-flag-testing | 461.0 KB | 187.7 KB |
| insight-actors | 562.1 KB | 187.7 KB |
| invite-email-preview | 456.0 KB | 187.7 KB |
| llm-costs | 559.5 KB | 187.7 KB |
| session-recording | 458.6 KB | 187.7 KB |
| session-summary | 463.9 KB | 187.7 KB |
| survey | 458.4 KB | 187.7 KB |
| survey-global-stats | 562.2 KB | 187.7 KB |
| survey-list | 565.0 KB | 187.7 KB |
| survey-stats | 562.2 KB | 187.7 KB |
| trace-span | 457.2 KB | 187.7 KB |
| trace-span-list | 564.2 KB | 187.7 KB |
| workflow | 457.1 KB | 187.7 KB |
| workflow-list | 563.7 KB | 187.7 KB |
| loops-review | 461.2 KB | 187.7 KB |
| query-results | 745.5 KB | 187.7 KB |
| render-ui | 826.2 KB | 187.7 KB |
| visual-review-snapshots | 461.6 KB | 187.7 KB |
⚠️ Backend coverage — 96.0% of changed backend lines covered — 23 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 96.0% (572 / 595)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/facade/loops.py |
88.2% | 451, 1037, 1040, 1052, 1057–1058, 1076–1077, 1118–1119, 1121, 1147–1148, 1179–1180, 1238, 1241–1242 |
products/tasks/backend/logic/services/loop_runs.py |
92.9% | 553, 594, 608, 629–630 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 30077710683 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
tasks |
██████████████░░░░░░ 69.8% |
30,618 / 43,882 |
signals |
████████████████░░░░ 80.0% |
22,090 / 27,597 |
cdp |
████████████████░░░░ 81.0% |
3,144 / 3,881 |
data_modeling |
█████████████████░░░ 82.7% |
5,547 / 6,708 |
notebooks |
█████████████████░░░ 85.3% |
7,275 / 8,531 |
agent_platform |
█████████████████░░░ 86.4% |
3,807 / 4,405 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
cohorts |
██████████████████░░ 87.8% |
4,488 / 5,114 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
exports |
██████████████████░░ 88.4% |
6,949 / 7,861 |
data_warehouse |
██████████████████░░ 88.9% |
11,887 / 13,378 |
conversations |
██████████████████░░ 89.3% |
16,875 / 18,894 |
engineering_analytics |
██████████████████░░ 89.4% |
6,386 / 7,145 |
dashboards |
██████████████████░░ 89.4% |
5,983 / 6,693 |
error_tracking |
██████████████████░░ 89.7% |
10,166 / 11,334 |
alerts |
██████████████████░░ 90.0% |
4,056 / 4,508 |
early_access_features |
██████████████████░░ 90.1% |
1,031 / 1,144 |
mcp_analytics |
██████████████████░░ 90.1% |
2,763 / 3,065 |
streamlit_apps |
██████████████████░░ 90.4% |
2,501 / 2,767 |
slack_app |
██████████████████░░ 90.7% |
9,028 / 9,951 |
marketing_analytics |
██████████████████░░ 91.0% |
11,792 / 12,964 |
stamphog |
██████████████████░░ 91.1% |
4,056 / 4,450 |
product_analytics |
███████████████████░ 92.6% |
5,850 / 6,320 |
ai_observability |
███████████████████░ 92.8% |
15,041 / 16,214 |
surveys |
███████████████████░ 93.0% |
5,730 / 6,163 |
web_analytics |
███████████████████░ 93.1% |
14,426 / 15,492 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
workflows |
███████████████████░ 93.6% |
6,390 / 6,826 |
endpoints |
███████████████████░ 94.1% |
8,640 / 9,177 |
review_hog |
███████████████████░ 94.7% |
6,814 / 7,199 |
skills |
███████████████████░ 94.7% |
3,300 / 3,483 |
logs |
███████████████████░ 95.4% |
10,012 / 10,498 |
experiments |
███████████████████░ 95.7% |
25,036 / 26,159 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
replay_vision |
███████████████████░ 96.2% |
15,260 / 15,864 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.5% |
17,110 / 17,734 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 97.1% |
9,047 / 9,317 |
warehouse_sources |
███████████████████░ 97.1% |
330,460 / 340,224 |
data_catalog |
███████████████████░ 97.4% |
2,367 / 2,429 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
⚠️ Django migration SQL — 1 new migration to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/tasks/backend/migrations/0072_loop_skill_bundles.py
BEGIN;
--
-- Add field skill_bundles to loop
--
ALTER TABLE "posthog_task_loop" ADD COLUMN "skill_bundles" jsonb DEFAULT '[]'::jsonb NOT NULL;
ALTER TABLE "posthog_task_loop" ALTER COLUMN "skill_bundles" DROP DEFAULT;
COMMIT;Last updated: 2026-07-24 08:09 UTC (0ff61e2)
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 0 Safe | 1 Needs Review | 0 Blocked
⚠️ Needs Review
May have performance impact
tasks.0072_loop_skill_bundles
└─ #1 ⚠️ AddField
Adding NOT NULL field with callable default (list) - verify it's stable
model: loop, field: skill_bundles, default: list
Last updated: 2026-07-24 08:09 UTC (0ff61e2)
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
|
🔀 Tried to auto-resolve conflicts with I won't retry until the branch or master moves. |
67b9567 to
9692f30
Compare
| # Freeze the bundle set on the run itself: seeding (post-commit and reconciler | ||
| # recovery alike) reads only this snapshot, so a later replace or ownership takeover | ||
| # can't change which skills this run installs under its captured credentials. | ||
| bundle_seeds = _snapshot_skill_bundle_seeds(loop) |
There was a problem hiding this comment.
High: Bundles persist across ownership takeover
A malicious owner can attach a bundle and leave the loop enabled. If a teammate then takes ownership, this code snapshots the old bundle while creating the task with the new owner's credentials. A scheduled or teammate-triggered fire can therefore run attacker-authored skill instructions with the new owner's authority. Clear and expire bundles during takeover, or bind each manifest to its owner and refuse to seed it after ownership changes until the new owner explicitly approves or replaces it.
c96f078 to
525e4de
Compare
Problem
posthog-code is adding "run a skill instead of instructions" to Loops. Cloud task runs already support skills as
skill_bundlerun artifacts, but a client bundles and uploads them at task creation. Loop fires happen server-side with no client attached, so a loop needs to store its skill bundles once and have every fired run seeded with them.Changes
Loop.skill_bundlesJSONField (migration 0071): manifest entries in the same shape asTaskRun.artifacts, bytes under a loop-scoped S3 prefix so run retention never reaps them.PUT /loops/{id}/skill_bundles/: wholesale replace. Verifies base64 and sha256, caps count (10) and size (30MB), writes to S3 and deletes superseded objects best-effort. Gated like other identity-bearing config:skill_bundlesjoinsIDENTITY_FIELDS, so on team loops only the owner may change it.fire_loopseeds each stored bundle into the new run: S3 copy into the run's artifact prefix plus a matchingskill_bundlemanifest entry. The sandbox agent-server already installs those, so no sandbox changes. A failed copy fails the fire, which rolls back and retries cleanly rather than running with silently missing skills.LoopSerializerexposesskill_bundles(skill name, source, size, sha256, uploaded_at; no storage paths).Client side: PostHog/code#3741.
How did you test this code?
pytest products/tasks/backend/tests/test_loops_api.py products/tasks/backend/tests/test_loop_runs.py(119 passed). New coverage, each catching a regression no existing test did:LoopSkillBundlesAPITest: replace + readback + clear (manifest and S3 delete of superseded objects), sha256 mismatch is a 400 with no S3 write, count cap, non-owner replace on a team loop is a 403, someone else's personal loop is a 404.TestFireLoopSeedsSkillBundles: a fire copies bundles into the run manifest under the run prefix, a bundle-less fire never touches storage, a failed S3 copy fails the fire with no orphaned run.makemigrations --checkclean, ruff and mypy clean on touched files.Automatic notifications
Docs update
N/A
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Design was chosen against two rejected alternatives: copying the skill body into
loop.instructions(loses companion files, drifts) and resolving team LLMSkills at fire time (needs a new skills-to-sandbox path; today nothing installs LLMSkills into sandboxes). Snapshot-at-save reuses the exactskill_bundleartifact mechanic the sandbox agent-server already consumes. A dedicated replace endpoint (instead of extending the loop write serializer) keeps multi-megabyte base64 payloads out of every PATCH and gives the client one idempotent call.